Skip to content

add validate domain option to electrum #155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
dspicher wants to merge 1 commit into from
Closed

add validate domain option to electrum #155

dspicher wants to merge 1 commit into from

Conversation

@dspicher
Copy link
Contributor

@dspicher dspicher commented Jun 9, 2023 •
edited
Loading

Description

Added the validate_domain option to Electrum arguments.

Since #151 seems dormant, I took another stab at this.

Fixes #134.

Changelog notice

Added the validate_domain option to Electrum arguments.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@dspicher dspicher changed the title Added validate_domain Electrum option Draft: add validate domain option to electrum Jun 9, 2023
@dspicher dspicher force-pushed the validate-domain branch 2 times, most recently from 985ff9f to c6c44e1 Compare June 9, 2023 08:47
@dspicher dspicher changed the title Draft: add validate domain option to electrum add validate domain option to electrum Jun 9, 2023
@notmandatory
Copy link
Member

notmandatory commented Jun 16, 2023

Take a look at the utils.rs file and on line 403 you need to use this new param you're adding.

notmandatory
notmandatory reviewed Jun 16, 2023
View reviewed changes
src/commands.rs
pub stop_gap: usize,

/// Enable domain validation when connecting to Electrum servers.
#[clap(name = "VALIDATE_DOMAIN", long = "validate_domain")]
Copy link
Member

@notmandatory notmandatory Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the utils.rs the default for this value is true, as you have it here the user will be required to set the value to true or false. It's better if you set the clap default_value to true then the user only needs to do something if they want it to be false.

Copy link
Contributor Author

@dspicher dspicher Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also added a v short option.

@dspicher
Copy link
Contributor Author

dspicher commented Jun 16, 2023

Take a look at the utils.rs file and on line 403 you need to use this new param you're adding.

Ah yeah, makes sense. Fixed.

@dspicher dspicher force-pushed the validate-domain branch 5 times, most recently from 719c23b to 976373d Compare June 16, 2023 14:44
notmandatory
notmandatory previously approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 976373d

@notmandatory notmandatory self-requested a review June 27, 2023 18:46
@notmandatory notmandatory dismissed their stale review June 27, 2023 18:47

found problem after approving

notmandatory
notmandatory requested changes Jun 27, 2023
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and realized the approach won't work since clap treats bool as a "flag" with no way to set to false if the default is true. See comments for how to fix.

src/commands.rs Outdated
)]
pub stop_gap: usize,

/// Enable domain validation when connecting to Electrum servers.
Copy link
Member

@notmandatory notmandatory Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to make it an Option and set the default in the code, see below.

Suggested change
/// Enable domain validation when connecting to Electrum servers.
/// Enable domain validation when connecting to Electrum servers [default: true]

Copy link
Contributor Author

@dspicher dspicher Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

src/commands.rs Outdated
#[clap(
name = "VALIDATE_DOMAIN",
long = "validate_domain",
default_value = "true"
Copy link
Member

@notmandatory notmandatory Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove default.

Suggested change
default_value = "true"

Copy link
Contributor Author

@dspicher dspicher Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

src/commands.rs Outdated
long = "validate_domain",
default_value = "true"
)]
pub validate_domain: bool,
Copy link
Member

@notmandatory notmandatory Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change type to Option:

Suggested change
pub validate_domain: bool,
pub validate_domain: Option<bool>,

Copy link
Contributor Author

@dspicher dspicher Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

src/utils.rs Outdated
timeout: wallet_opts.electrum_opts.timeout,
stop_gap: wallet_opts.electrum_opts.stop_gap,
validate_domain: true,
validate_domain: wallet_opts.electrum_opts.validate_domain,
Copy link
Member

@notmandatory notmandatory Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default to true here:

Suggested change
validate_domain: wallet_opts.electrum_opts.validate_domain,
validate_domain: wallet_opts.electrum_opts.validate_domain.unwrap_or(true),

You'll also need to fix the corresponding tests.

Copy link
Contributor Author

@dspicher dspicher Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@notmandatory
Copy link
Member

notmandatory commented May 28, 2025

This is old and needs to be rebased. Happy to re-open if author would like to update it.

@github-project-automation github-project-automation bot moved this from In Progress to Done in BDK-CLI May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@notmandatory notmandatory notmandatory requested changes

Assignees

@dspicher dspicher

Labels

None yet

Projects

BDK-CLI
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add validate_domain Electrum option

2 participants

@dspicher @notmandatory